fix(extract): prevent duplicate Function nodes for Java interface/enum methods (#1234) - #1327
Conversation
|
Thank you for this — and thank you for the sibling PR #1329, which I merged earlier tonight (the GoogleTest unique-name fix, The failing checks here are genuinely yours, which is worth saying plainly because it is not true of most of the red PRs I have been through tonight — several were cancellation noise or main-side breakage. This one is not. The signature is unambiguous: the same assertion fails deterministically on all five platforms — macOS 14, macOS 15 Intel, Ubuntu latest, Ubuntu arm64 and Windows CLANG64 all fail identically, while other PRs' runs pass that suite on comparable merge refs. One cause, five legs, no flake pattern. What is happening. The dedup in The fix shape: re-attach the edges to the surviving node before deduplicating, rather than dropping a node that still owns relationships. Deduplication in this graph has to be a merge, not a delete — otherwise it trades one accuracy bug (a spurious duplicate node) for a worse one (a missing To reproduce locally in seconds, rather than waiting on CI: That is the smallest arena that shows the failure, so you can iterate on it directly and only widen once it is green. Worth adding a test alongside the fix that asserts the surviving node still carries its The underlying change is a real improvement and closes a genuine issue (#1234); it is only the edge-preservation half that is missing. Ping me when it is pushed and I will pick it straight back up. |
…hods (DeusData#1234) Signed-off-by: Harshita Joshi <j.harshitaa06@gmail.com>
34bb44a to
0cde6ba
Compare
|
@DeusData Pushed the fix. You were right, the dedup needed to be a merge, not a delete. Rewrote it as a post-extraction pass that transfers call records to the surviving Method node before removing the duplicate Function. Also added |
…g CALLS edges (DeusData#1234) Signed-off-by: Harshita Joshi <j.harshitaa06@gmail.com>
0cde6ba to
11ee490
Compare
|
Thanks for the fast turnaround — this round holds up, and I traced the whole path rather than taking the description at its word. What actually fixes the edge is not the part you'd expect. The There's a genuinely nice side effect you didn't claim. Eight other record kinds carry The new probe binds properly. Two asks before merge:
And one thing that is on me, not you. My round-1 wording — "the dedup must merge, not delete" — plausibly steered you away from your own commit 1, which was the root-cause fix: it stopped the spurious So: please don't rework anything yet. Do the two small asks above, and I'll come back with the architecture answer. Nothing you shipped is defective — this is a question about which of two correct shapes we want to live with. |
Signed-off-by: Harshita Joshi <j.harshitaa06@gmail.com>
|
@DeusData Done! Updated the PR description to document the dedup-pass design and dropped the non-binding |
|
Both asks done, thank you — and yes, keep the new title. It describes what the diff actually does, which is the only thing a title owes anyone. Dropping the Nothing further is needed from you right now. The remaining question is the architecture one I flagged — prevention at the source versus the post-hoc dedup pass — and that is genuinely the maintainer's to answer, not a defect in what you shipped. I have put it to him with both sides written out, including the argument for your current shape: I will come back to you with the answer. If it goes the prevention way, the work is mostly un-reverting your own commit 1, so nothing you have done is wasted either way. |
|
The maintainer has decided, and the answer is your original instinct: prevention at the source, not the post-hoc pass. Which means my round-1 wording sent you the wrong way, and I am sorry for the round trip. What we would like, concretely:
Why the maintainer went this way, since you put real work into the other shape and deserve the reasoning: The dedup pass was unconditional across all 159 languages. Its job is to delete nodes we should not have created — which means that from the moment it exists, any future double-extraction bug in any language gets silently swept up instead of failing a test. That is a permanent reduction in what our tests can catch, in exchange for fixing one language's bug. The objection I originally raised against prevention — that And your original commit was closer to right than my feedback implied. "Merge, not delete" was the correct instruction for a dedup pass; it was the wrong instruction because a dedup pass was not the right shape. That distinction is on me, not on you. Thank you for the patience — you have now turned this around twice within the hour each time, and both rounds improved the change. |
…lass_body_children Signed-off-by: Harshita Joshi <j.harshitaa06@gmail.com>
|
@DeusData Pushed. Restored the prevention line in |
What does this PR do?
Fixes #1234.
Java interface and enum methods were emitted as both a Method node (correct, via
extract_class_methods) and a duplicate Function node (incorrect, via thewalk_defsfallback).Two changes fix this:
Prevention in
push_class_body_children: addsinterface_bodyandenum_bodyto the body-container check, gated toCBM_LANG_JAVAso grammars that share these node names (TypeScript, Apex, Dart) are unaffected. Follows the same pattern as the existing Groovyclosuregate. This stops the fallback path from re-walkingmethod_declarationchildren as top-level functions.extract_class_methodsdescends intoenum_body_declarations: Java enums wrap methods inside this node. Without this, enum methods never received proper class-qualified Method definitions. This also fixes a side effect on main where eight other record kinds carryingenclosing_func_qn(usages, throws, read/write, type refs, env accesses, type assigns, string refs, channels) were dangling for enum method bodies.Verification
build/c/test-runner extraction-- 266 passedbuild/c/test-runner convergence_probe-- 48 passedjava_interface_no_duplicate_function_issue1234andjava_enum_dedup_preserves_calls_issue1234assert Method nodes survive and Function count is zerocp_interface_method_no_dup_functionasserts surviving Method node carries CALLS edgesChecklist
git commit -s)make -f Makefile.cbm test)ASan hangs on macOS Tahoe 26.5 (Apple Clang 17). Full suite passes when built without sanitizers (
SANITIZE="").make -f Makefile.cbm lint-ci)